Don't store plaintext passwords #1040

gdsmith · 2025-05-22T16:39:52Z

Rather store hashed passwords in the way mysql server does.

here we:

update CredentialProvider to return new Credential struct
Credential includes the plugin that the user was created with
update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible
update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available
rework the password negotiation to switch plugin type to match the stored credentials
add hashing and comparison functions for the above where missing from existing libraries

Rather store hashed passwords in the way mysql server does. here we: - update CredentialProvider to return new Credential struct - Credential includes the plugin that the user was created with - update InMemoryProvider to handle hashing of passwords and add default auth method to make usage backwards compatible - update server authentication to use mysql server methods of comparing hashes rather than relying on having the plaintext password available - rework the password negotiation to switch plugin type to match the stored credentials - add hashing and comparison functions for the above where missing from existing libraries

gdsmith · 2025-05-22T16:44:23Z

Code is fully working as much as I can see but I am seeing failures with the TestCachingSha2Cache tests. I don't fully understand if it's a client problem or a change I've introduced means the server is pulling those credentials multiple times when it didn't previously. I did try debugging and it seemed like the cache was empty even if I confirmed the cache was actually written. Would love some feedback, and some help with the failing tests if at all possible.

also update the caching test to allow for the fact that we request the password once on every auth attempt

gdsmith · 2025-05-23T08:27:55Z

The changes to the code is what has broken the test, we are now getting the password to confirm the auth method. I think the code changes break the way you are checking if we are using the cache response, but I'm not sure how to update the test to ensure we are using the cache (though through debugging I can confirm that it does)

Copilot

Pull Request Overview

Avoid storing plaintext passwords by hashing credentials and updating authentication flow to match MySQL server behavior.

Change CredentialProvider and InMemoryProvider to return and store a Credential struct containing a hashed password and plugin name.
Refactor server handshake and auth logic to use hashed credentials, plugin negotiation, and unified compareAuthData.
Add utility functions in mysql/util.go for password hashing, encoding/decoding, and plugin-specific comparisons.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
server/server_test.go	Adapt tests for new `AddUser` signature and `NewCustomizedConn`
server/resp.go	Track `authPluginName` on switch requests
server/handshake_resp.go	Use `credential.authPluginName` for auth switch logic
server/credential_provider.go	Return `Credential` (hash+plugin), hash on add, preserve defaults
server/conn.go	Replace plaintext `password` field with `Credential`
server/caching_sha2_cache_test.go	Update call count assertion and `NewCustomizedConn` usage
server/auth_switch_response.go	Simplify to `compareAuthData`
server/auth.go	Refactor all comparisons to use `Credential`
mysql/util.go	Add native, SHA2, SHA256 hashing + encode/decode helpers
client/auth.go	Update client to use new hash util signatures

Comments suppressed due to low confidence (4)

server/auth_switch_response.go:11

The import 'github.com/pingcap/tidb/pkg/parser/auth' is unused in this file and should be removed to avoid compile errors.

"github.com/pingcap/tidb/pkg/parser/auth"

mysql/util.go:216

rand.Read is used here but package 'crypto/rand' is not imported; either import 'crypto/rand' or qualify this call.

_, err := rand.Read(salt)

mysql/util.go:90

strings.ToUpper is used in EncodePasswordHex but the 'strings' package is not imported; add import "strings".

hexstr := strings.ToUpper(hex.EncodeToString(passwordHash))

server/auth.go:106

compareNativePasswordAuthData accepts a Credential parameter but decodes c.credential.password instead of the passed-in credential; change to use credential.password.

password, err := mysql.DecodePasswordHex(c.credential.password)

lance6716 · 2025-05-25T10:00:05Z

mysql/util.go

-	crypt.Write(hash)
-	scramble = crypt.Sum(nil)
+	crypt.Write(stage2)
+	scrambleHash := crypt.Sum(nil)


nit: seems stage2 is not used anymore, so we can pass-in stage2[:0] to reuse the array under the slice.

lance6716 · 2025-05-25T10:00:47Z

mysql/util.go

+	return Xor(scrambleHash, stage1)
+}
+
+func Xor(hash1 []byte, hash2 []byte) []byte {


please add function comment that hash1 will be modified in-place.

lance6716 · 2025-05-25T10:03:20Z

server/auth.go

-		}
-		if bytes.Equal(plain, dbytes) {
-			return nil
+		clientAuthData = mysql.Xor(dbytes, c.salt)


(I need more time to check if the lengths of dbytes and c.salt are equal)

lance6716 · 2025-05-25T10:04:32Z

mysql/util.go

+}
+
+// hash_stage1 = xor(reply, sha1(public_seed, hash_stage2))
+func Stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {


Seems this function does not need to be public. It can save a little time to search for its caller outside of the package

Suggested change

func Stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {

func stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {

lance6716 · 2025-05-25T10:09:38Z

mysql/util.go

+	return Xor(scramble, seededHash)
+}
+
+// FROM vitess.io/vitess/go/mysql/auth_server.go


Thanks for vitness. And in addition to its link, I think it's better to memtion this doc https://dev.mysql.com/doc/mysql-security-excerpt/5.7/en/password-hashing.html

"Password hashes in the 4.1 format always begin with a * character"

lance6716 · 2025-05-25T10:15:31Z

mysql/util.go

@@ -135,6 +202,91 @@ func EncryptPassword(password string, seed []byte, pub *rsa.PublicKey) ([]byte,
 	return rsa.EncryptOAEP(sha1v, rand.Reader, pub, plain, nil)
 }

+const (
+	SALT_LENGTH                = 16


nit: My IDE says "Use camel case instead of snake case". However this project has both style so you can ignore it.

lance6716 · 2025-05-25T10:19:27Z

mysql/util.go

+
+// hashCrypt256 salt and hash a password the given number of iterations
+func hashCrypt256(source, salt string, iterations uint64) (string, error) {
+	actualIterations := iterations * ITERATION_MULTIPLIER


for this function, the const ITERATION_MULTIPLIER is directly used. But for generateUserSalt SALT_LENGTH is pass-in as an argument. I think generateUserSalt can also directly use SALT_LENGTH

lance6716 · 2025-05-25T10:22:35Z

mysql/util.go

+	for i := uint64(0); i < actualIterations; i++ {
+		h := sha256.New()
+		h.Write(hashInput)
+		hash = sha256.Sum256(h.Sum(nil))


By checking the source code of sha256.Sum256 I see it's the same as h.Write(hashInput) and h.Sum(nil). Did you wrongly repeat the process?

lance6716 · 2025-05-25T10:27:13Z

mysql/util.go

+	}
+
+	hashHex := hex.EncodeToString(hash[:])
+	digest := fmt.Sprintf("$%d$%s$%s", iterations, salt, hashHex)


In generateUserSalt I see some restrictions are added to the random bytes. Is this line the reason of it? I think it's OK to remove the restrictions to make the whole logic more simple. We can hex-encode the salt without the restrictions.

lance6716 · 2025-05-25T10:28:47Z

mysql/util.go

+		return false, err
+	}
+
+	return bytes.Equal(pwhash, []byte(newHash)), nil


subtle.ConstantTimeCompare

lance6716 requested a review from dveeden May 23, 2025 01:41

Update to ensure we only request the password once

43cc679

also update the caching test to allow for the fact that we request the password once on every auth attempt

Merge branch 'master' into compare-hashes

4e6aea2

lance6716 requested a review from Copilot May 25, 2025 09:53

Copilot AI reviewed May 25, 2025

View reviewed changes

lance6716 reviewed May 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't store plaintext passwords #1040

Don't store plaintext passwords #1040

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

lance6716 May 25, 2025

Uh oh!

Uh oh!

	func Stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {
	func stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {

Don't store plaintext passwords #1040

Are you sure you want to change the base?

Don't store plaintext passwords #1040

Conversation

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 22, 2025

Uh oh!

gdsmith commented May 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!